Conversation
|
Hi Paul, Thanks for your work on the IXL driver! So let's get started to prepare your changes for being merged into the mainline version! Unfortunately, your changes break IXL right in the first commit, as the constant So before looking into the code in more detail, these would be the first things to address. Once this is done, we can move further. |
|
Hi Till, already noticed the missing definition for the igb on monday but forgot to push it 😅. It should be fixed with commit 82acbb4. I will fix the 80 character limit per line but other than that the code is identical with the code of the ixl igb. It should therefore already adhere to the contributing style guides. I will also have look at the warnings and unused functions. Thanks for your feedback. Best Regards Paul |
|
Hi Paul, Could you update this PR in a way that the repository remains bisectable? I.e., I would like each commit to leave the code base in a state that allows for compiling it without errors and warnings. I particular this means that you would have to amend the first two commits with the fixes you committed later on. Thanks! |
da1be6a to
5f5722d
Compare
|
Hi Till, This should now compile without problem and it is unified into one build-able commit. I am curious about your feedback about the coding style as the igc driver is mainly copy of the ixl igb driver, which should already adhere to the coding style. Best Regards Paul |
|
Hi Paul, Unfortunately, the first commit still does not compile, since the constant Thanks! |
5f5722d to
78f631c
Compare
|
The first commit is now also squashed into unified commit containing the i210 support for the igb as well as the igc driver code. I also added the igc and its supported nics to the readme. |
tmiemietz
left a comment
There was a problem hiding this comment.
Hi Paul,
A first review of the proposed code changes is now complete. I did not go through all of the code yet. Nevertheless, could you please fix the issues brought up in the comments and also adapt the remainder of your PR in a way that it aligns with the style of the existing code base? I'll then give it another pass.
Thanks!
| @@ -0,0 +1,225 @@ | |||
| /** | |||
There was a problem hiding this comment.
Could you follow the formatting of the other header files? The first comment block should have a one-line description on what the file does, along with your name and the year(s) in that the file was modified. Also dump all of the information of version, date, file. Have a look at e.g. igb.h. The same goes for the layout of comment blocks (where each line should be preceded with an asterisk.
|
|
||
| #include <stdbool.h> | ||
| #include <l4/ixl/stats.h> | ||
|
|
There was a problem hiding this comment.
Delete that newline. The structure of includes is , , local header files with each category being separated by a newline.
|
|
||
| #pragma once | ||
|
|
||
| #include <stdbool.h> |
There was a problem hiding this comment.
Blank line after this include. Also, what is the reason for including stdbool.h here?
|
|
||
|
|
||
|
|
||
| /*** Functions ***/ |
There was a problem hiding this comment.
Could you center these intermediate headings, with the righthand delimiter being aligned to column 80? In this way, the code looks way more fancy :)
| /** | ||
| * Disables an MSI for receive events. | ||
| * | ||
| * \param qid Index of the RX queue for that the corresponding MSI-X shall |
There was a problem hiding this comment.
For these multi-line doxygen statements: Could you align the descriptive text so that all lines of it start in the same column? Or does this break doxygen?
| return 0; | ||
| } | ||
|
|
||
| if(status & IGC_STATUS_SPEED_2500) |
There was a problem hiding this comment.
Again spacing after keywords.
| if (status & IGC_STATUS_FD) { | ||
| duplex = FULL_DUPLEX; | ||
| //ixl_debug("Full Duplex\n"); | ||
| } else { |
| status = get_reg32(baddr[0], IGC_STATUS); | ||
| if (status & IGC_STATUS_FD) { | ||
| duplex = FULL_DUPLEX; | ||
| //ixl_debug("Full Duplex\n"); |
There was a problem hiding this comment.
Drop the comments if they are debugging artifacts.
|
|
||
| struct mac_address Igc_device::get_mac_addr(void) { | ||
|
|
||
| ixl_info("Entering..."); |
There was a problem hiding this comment.
Drop those debugging things altogether.
There was a problem hiding this comment.
I have reviewed at least the first half of the file. Even though the style guide in CONTRIBUTING.md is not complete: Could you please take a look at existing source files and adhere to the coding style you find in them? This holds e.g. for the placement of the else keyword / placement of opening braces, spacing rules, or adhering to the code boundary of 80 columns. Also please mind the proper naming of variables. Debugging code should be removed.
Developed and tested the igc driver as part the bachelor thesis: "Generic aspects of porting a Linux Ethernet driver to the L4Re microkernel". The bachelor thesis will probably published in the coming month.
Also tested the igb driver with the i210 NIC and added support for it.